Skip to content

Do not assign offsetgroup to traces in px unless barmode="group" #4865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Nov 18, 2024

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented Nov 12, 2024

Closes #4863

This change is required due to the change in behavior for offsetgroup introduced in Plotly.js 3.0: plotly/plotly.js#7009

The above change causes bars to appear as a group if offsetgroup is set, even if barmode is set to stacked, which changes the default behavior of histograms and bar charts, as explained in #4863 (see comment).

This PR fixes the issue by only setting offsetgroup if barmode="group". (We also set offsetgroup if barmode is not set, since group is the default barmode in Plotly.js.)

Note that we can't remove setting offsetgroup entirely due to the issue described in my comment here.

The other change (changing the point at which we set barmode to overlay if the figure has marginal histograms) is so that the figure's barmode in that case will be set before the point where we decide whether to set offsetgroup.

@emilykl
Copy link
Contributor Author

emilykl commented Nov 12, 2024

Unfortunately, not using offsetgroup at all breaks an edge case for faceted charts, as demonstrated by the percy tests histogram_histfunc and bar_facet.

Without offsetgroup, when there is no data in a facet for one bar in a group of bars, the other bars expand to fill the available space.

See comparison below — I believe the behavior on master is the way that this chart should be rendered.

master fix-bar-charts (this branch)
Screen Shot 2024-11-12 at 1 34 44 PM Screen Shot 2024-11-12 at 1 34 55 PM
Screen Shot 2024-11-12 at 1 42 17 PM Screen Shot 2024-11-12 at 1 40 45 PM

@gvwilson gvwilson added P1 needed for current cycle fix fixes something broken labels Nov 13, 2024
@emilykl emilykl changed the title Do not assign offsetgroup to traces in px for bar, violin, box, and histogram traces Do not assign offsetgroup to traces in px unless barmode="grouped" Nov 14, 2024
@emilykl emilykl changed the title Do not assign offsetgroup to traces in px unless barmode="grouped" Do not assign offsetgroup to traces in px unless barmode="group" Nov 14, 2024
@emilykl emilykl marked this pull request as ready for review November 14, 2024 22:34
@emilykl
Copy link
Contributor Author

emilykl commented Nov 14, 2024

Note: The 4 visual changes in Percy are the 4 charts which were incorrect on master and are now returning to how they should look.

@gvwilson
Copy link
Contributor

thanks @emilykl - @marthacryan please review

Copy link
Collaborator

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@emilykl emilykl merged commit 84f4e66 into master Nov 18, 2024
4 of 5 checks passed
@emilykl emilykl deleted the fix-bar-charts branch November 18, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix fixes something broken P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some charts rendering differently on master branch vs Plotly.py 5.24.1
3 participants